fix: one unloadable grammar no longer takes down the whole CLI (#323) - #337
fix: one unloadable grammar no longer takes down the whole CLI (#323)#337Ercaner1988 wants to merge 1 commit into
Conversation
🌱 graft blast radius1 area changed → 8 areas can be affected. 41 dependent symbols, depth 2. flowchart TB
A0(("Graph Construction<br/>16 symbols"))
A1(("Review Processing<br/>10 symbols"))
A2(("CLI Engine<br/>6 symbols"))
A3(("MCP Tools<br/>4 symbols"))
A4(("Language Enrichment<br/>2 symbols"))
AX(("3 smaller areas<br/>3 symbols"))
classDef reached fill:#D9EDF3,stroke:#3AA7C9,stroke-width:1.5px,color:#0E313C;
class A0,A1,A2,A3,A4 reached;
classDef tail fill:#EEF2F3,stroke:#9AA4A9,stroke-width:1px,color:#3A4247;
class AX tail;
Who knows this code — 4 people across 9 areas
Ownership is git history over each area's own files, weighted towards recent work (120-day half-life). Merge commits and bots are dropped, and you are dropped from your own PR. A name with no All 41 dependent symbols, grouped by areaGraph Construction — 16 symbols in 8 files
Review Processing — 10 symbols in 6 files
CLI Engine — 6 symbols in 2 files
MCP Tools — 4 symbols in 1 file
Language Enrichment — 2 symbols in 1 file
Viewer Build — 1 symbol in 1 file
Context Build — 1 symbol in 1 file
Sync Execution — 1 symbol in 1 file
Test signal per changed area — 1 ⚠Reached = a node under a test path has a resolved edge into the changed symbol. It undercounts anything called indirectly — through a CLI, a spawned process or a dynamic import — so read a low ratio as “look here”, never as a coverage gate.
39 test suites also reference this code48 symbols, kept out of the diagram and the table so they cannot crowd out the areas a reviewer has to look at.
Open the interactive graph → — click an area to see its dependent symbols at file:line. |
faa9ee5 to
05c74b3
Compare
05c74b3 to
2b42289
Compare
extract.ts imported all nine depth-tier grammars at the top of the module. They are native (node-gyp) modules, so any one of them failing to load took the whole CLI down at import time, before argv was read — `--version` and `--help` died exactly like a build, with a node-gyp-build stack trace that never says "graft". On the machine in trailhq#323 that grammar is tree-sitter-kotlin, which ships no prebuilds at all and so cannot load without a C toolchain. Load them through createRequire instead and keep what loads. The extension table is filtered by that, which is all the rest of the pipeline needs: an unavailable language stops claiming its files, and they take the paths a language graft has no grammar for takes today — the breadth tier where a generic row claims the extension, unindexed where none does. Warned once per language, the first time a file it would have claimed comes past, so a repo with no Kotlin in it stays quiet and a Kotlin repo never indexes short in silence. That alone keeps the CLI alive but leaves a language with no breadth-tier row indexing nothing — and Kotlin had none, so the machine in trailhq#323 would start and index no Kotlin at all. tree-sitter-wasm ships a kotlin grammar and queries/kotlin.scm has been in the tree unused since Kotlin was promoted to the depth tier, so the fallback is one registry row. A row whose extension the depth tier also claims is unreachable while the native grammar loads — `.java` has been exactly that all along, which is why the "must not collide" note needed rewriting rather than an exception. Both warmGenericGrammars calls now apply the tier precedence their own parse loops apply, so a fallback row cannot warm a WASM grammar the run will never call. That was already true of `.java`; a second row would have doubled it. Measured on a three-file repo (.ts/.kt/.rs) with tree-sitter-kotlin broken: the CLI does not start before this change; with the isolation alone, 6 nodes [rust, typescript]; with the fallback row too, 11 nodes [kotlin, rust, typescript]. With the grammar healthy the graph is identical either way. The test breaks a grammar both ways a module can be reached, `import` and `require`, so it still fails if the static imports ever come back. Refs trailhq#323 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
2b42289 to
df222ae
Compare
Ercaner1988
left a comment
There was a problem hiding this comment.
well, I rechecked our implementation and couldn't find any blatant error or anything.
actionlint checks context availability, expression syntax, action inputs, and shellchecks `run:` blocks -- none of which YAML validation catches. Soup's CI found this the hard way (a job-level `env:` referencing the `runner` context, valid YAML, invalid Actions, dead before any job logged) and pins the release binary by SHA-256 rather than `go install`-ing it. Ported verbatim, credited. Also lands test/ratchet-lazy-grammar-import.test.ts: an AST-based repo-wide ratchet banning a static top-level import of any native tree-sitter grammar package (trailhq#323) -- the exact idiom trailhq#337 and trailhq#217 both moved away from. It runs as an ordinary test under ci.yml's existing `npm test`, no new job needed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… depth language (trailhq#337 trailhq#217 trailhq#214 trailhq#325) Four PRs were rewriting the same code path in extract.ts — one function now does what they each asked for: - Lazy loading (trailhq#217): a grammar (and the core tree-sitter binding itself) is required at most once, the first time entryFor()/depthExtensions() is asked about that language, not for all nine at module-import time. `graft mcp` never asks before the client's `initialize` reply, so a client can no longer see that handshake stall behind nine native loads. - Per-language isolation (trailhq#337, unchanged): a grammar that will not load costs its own language, not the CLI. - WASM fallback for every depth language, not just Kotlin/Java (trailhq#214): the breadth tier already had a "FALLBACK row" mechanism (a GENERIC_LANGS row reachable only when the matching depth grammar failed); tree-sitter-wasm ships a .wasm for all nine depth languages already, so the remaining seven now have one too. None has a queries/<name>.scm, so on the rare machine that actually reaches one it degrades to the node-kind walker (symbols only) instead of leaving the language unindexed. - optionalDependencies (trailhq#325's core ask): the eight native grammar packages moved out of dependencies, so a platform lacking a prebuild for one no longer fails `npm install` for the other eight (core tree-sitter stays required — a missing core is a bigger question than one language, left for a follow-up). New test: `graft mcp` answers `initialize` with tree-sitter-typescript broken and never touches grammar loading at all (no warning on stderr) — the concrete claim trailhq#217 exists for. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Implements suggestion (3) from #323, plus (2) for Kotlin.
What happens today
src/graph/extract.tsimports all nine depth-tier grammars at the top of the module. They are native (node-gyp) modules, so any one of them failing to load takes the CLI down at import time, before argv is read — with anode-gyp-buildstack trace that never says "graft".On current
main, withtree-sitter-kotlinmade unloadable:I have hit this twice from unrelated causes: Kotlin, as in this issue, and
tree-sitter-runder Bun, where the binding asks for a prebuild filename that the scoped package does not ship (r-lib/tree-sitter-r#205, one-line fix in r-lib/tree-sitter-r#206). Both were one line to fix once found, and neither was findable from the error, which is the part fix (3) changes.The isolation (fix 3)
createRequireplus a try/catch per grammar, keeping whatever loads. The extension table is filtered by that, which is all the rest of the pipeline needs:entryForstops claiming the language's extensions, and its files then take the paths a language graft has no grammar for takes today — the breadth tier where a generic row claims the extension, unindexed where none does.listSourceFilesnever enumerates them, sobuild.ts'sgeneric!stays safe.The warning is once per language, from
entryFor— the first time a file that language would have claimed comes past. A repo with no Kotlin in it stays quiet; a Kotlin repo never indexes short in silence:This is the contract your two WASM tiers already state for themselves — "an unavailable grammar is skipped rather than fatal" (container.ts), "degrades to a file node only (never throws)" (generic.ts). The native tier was the one that could not say it.
tree-sitteritself stays a static import: it is the shared dependency of all nine, and there is no depth tier without it.Kotlin's fallback row (fix 2)
If you would rather take the isolation without this, it is the one
GENERIC_LANGSrow ingeneric.tsplus the two warm-call filters inbuild.tsandcheck.ts— say so and I will split it back out.With the isolation alone that machine starts but indexes no Kotlin at all, which on a Kotlin repo is still a bad day.
tree-sitter-wasmships a kotlin grammar andsrc/graph/queries/kotlin.scmhas been in the tree unused since Kotlin was promoted to the depth tier, so the fallback costs one registry row.A row whose extension the depth tier also claims is unreachable while the native grammar loads —
.javahas been exactly that all along. BothwarmGenericGrammarscalls now apply the tier precedence their own parse loops apply, so a fallback row cannot warm a WASM grammar the run will never call (already true of.java; a second row would have doubled it).Measured on a three-file repo (
.ts,.kt,.rs):tree-sitter-kotlin11 nodes, 8 edges [kotlin, rust, typescript]main6 nodes, 4 edges [rust, typescript], plus the warning11 nodes, 5 edges [kotlin, rust, typescript]On a healthy install the graph is identical with and without the fallback row — same nodes, same edges, compared field by field. The row really is unreachable.
graft checkagrees with the build it follows on the fallback path (graph check: OK, withRepo,describe,reloadandopenRepoall present), so this does not reopen the tier-mismatch shape of #236.Tests
test/grammar-unavailable.test.tsdrives the real CLI in a child process, withtest/break-grammar-preload.cjsstanding in for the missing native build — so it runs on a runner that has a compiler, which is why CI never caught the original. The preload breaks both ways a module can be reached,importandrequire, so the test still fails if the static imports ever come back. Each of the three fails without the part of the change it covers.npm teston this branch: 1223 tests, 1210 pass, 8 fail, 5 skipped — the same 8 that fail onupstream/mainbefore it, and no others. They aretoLocaleString()assertions that assume a,thousands separator, where thistr-TRmachine renders~100.000. Nothing to do with this PR: they are #338, fixed separately in #345, and this branch is deliberately independent of that one.The three tests this PR adds pass; each fails without the part of the change it covers.
One thing you may see and should not blame on this branch:
test/mcp-server.test.tstimed out on two of my five full-suite runs (itsrpc()helper waits a fixed 15s, then readsundefined.result). It is a load-sensitive deadline — those tests need ~2s to get a first response when run alone, and I measured MCP server startup identical on this branch and onmain(1.7–2.0s across five spawns each), so nothing here made the server slower. But this PR does add a test file that spawns three short-lived CLI processes, so it adds parallel load, and that deadline has little headroom on a busy machine.Environment: Node v24.16.0, Windows 11 x64, rebased onto
4f39d19(0.18.0 plus the brain commits after it).No CHANGELOG entry
I dropped the one I had. 0.18.0 is already released, so there is no unreleased section to add to, and
de8456eshows the release commit is what writes them. Happy to add a bullet wherever you want it.Not in this PR
Loading the nine grammars costs ~220 ms, paid by every command including
--version.GRAMMAR_MODULESmakes lazy per-language loading a small follow-up, but that is a performance change and does not belong in a fix.🤖 Generated with Claude Code